Reduce nesting depth in tests to respect server CQL limit#899
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR reduces integration-test nesting depths to match Scylla’s CQL expression nesting limits: the nested-tuple test in tests/integration/standard/test_types.py now uses a maximum depth of 11, and two nested-UDT tests in tests/integration/standard/test_udts.py now use max_nesting_depth = 10. Each updated test includes comments documenting how Scylla’s parser enforces nesting-depth limits. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Lorak-mmk I made a mistake and tested locally with wrong scylla version, thats why I choose wrong limit and we need a follow up PR, sorry for that |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/standard/test_udts.py (1)
425-428: ⚡ Quick winKeep the prepared-only UDT test above the parser limit to preserve coverage.
Line 425 introduces a prepared-only path, which is explicitly not constrained by parser nesting. Setting
max_nesting_depth = 10here drops verification of that contract. Consider keeping this test at 12+ (or splitting into a dedicated “prepared bypass” case) while retaining 10 for simple-literal paths.Suggested change
- # This test inserts via prepared statements (serialized values), so - # it is not subject to the parser's nesting limit. Keep it aligned - # with the literal-insert tests for consistency. - max_nesting_depth = 10 + # This test inserts via prepared statements (serialized values), so + # it is not subject to the parser's nesting limit. Keep this above + # the literal limit to preserve coverage of that behavior. + max_nesting_depth = 12🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/standard/test_udts.py` around lines 425 - 428, The test sets max_nesting_depth = 10 which moves the prepared-only UDT test below the parser nesting limit and loses coverage of the "prepared bypasses parser limit" behavior; update the test so the prepared-only path remains above the parser limit (e.g., set max_nesting_depth = 12 or split into two cases: one with max_nesting_depth = 10 for literal-insert behavior and a separate prepared-only test with max_nesting_depth >= 12) and ensure the prepared-path still exercises the bypass scenario (reference the max_nesting_depth variable in the UDT test).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/standard/test_udts.py`:
- Around line 425-428: The test sets max_nesting_depth = 10 which moves the
prepared-only UDT test below the parser nesting limit and loses coverage of the
"prepared bypasses parser limit" behavior; update the test so the prepared-only
path remains above the parser limit (e.g., set max_nesting_depth = 12 or split
into two cases: one with max_nesting_depth = 10 for literal-insert behavior and
a separate prepared-only test with max_nesting_depth >= 12) and ensure the
prepared-path still exercises the bypass scenario (reference the
max_nesting_depth variable in the UDT test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba261fdb-fa4d-4cb3-af57-51d91d5705c9
📒 Files selected for processing (2)
tests/integration/standard/test_types.pytests/integration/standard/test_udts.py
Scylla now limits CQL expression nesting depth to 12 (CVE-2026-31948,
scylladb commit e35c388), rejecting deeper literals with the error
"expression nested too deeply".
The limit counts every recursive `term`, including the innermost
scalar value:
- nested tuple literals max out at 11 levels deep
- nested UDT literals max out at 10 levels deep (a UDT literal
{value: ...} adds two term levels per nesting)
Adjust test_can_insert_nested_tuples to depth 11 and the nested UDT
tests to depth 10.
sylwiaszunejko
force-pushed
the
fix-nesting
branch
from
284425f to
823d13c
Compare
|
@Lorak-mmk could you reapprove? |
|
Not really, Github doesn't let me, the page loads indefinitely. |
Lorak-mmk
left a comment
Lorak-mmk
left a comment
There was a problem hiding this comment.
Oh, I managed to do it.
Why are some depths 11, and some 10?
scylla's implementation of checking lvls, it treats inside of a tuple as 1 lvl, but udt as 2 lvls |
2 participants
Scylla now limits CQL expression nesting depth to 12 (CVE-2026-31948, scylladb commit e35c388), rejecting deeper literals with the error "expression nested too deeply".
The limit counts every recursive
term, including the innermost scalar value:Prepared statements serialize values and bypass the parser limit, so only simple-statement/literal inserts are affected.
Adjust test_can_insert_nested_tuples to depth 11 and the nested UDT tests to depth 10.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.